[txpool] fix #4215, support allowed transaction list. #4218

peekpi · 2022-07-01T10:26:27Z

Issue

Config

cmd flag

--txpool.allowedtxs="./.hmy/allowedtxs.txt"

config file

[TxPool]
  AllowedTxsFile = "./.hmy/allowedtxs.txt"

AllowedTxFile format

Each line is in the following format:
from -> to : data

from is the sender account address, which can be ONE or ETH address.
to is the receiver account address, which can be ONE or ETH address.
data is the payload of the transaction, and 0x means empty.

example

0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f->0x855Ac656956AF761439f4a451c872E812E3900a4:0x
0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f->one1np293efrmv74xyjcz0kk3sn53x0fm745f2hsuc:0xa9059cbb
one1s4dvv454dtmkzsulffz3epewsyhrjq9y0g3fqz->0x985458E523dB3d53125813eD68c274899e9DfAb4:0xa9059cbb
one1s4dvv454dtmkzsulffz3epewsyhrjq9y0g3fqz->one10fhdp2g9q5azrs2ukk608x6krd4rleg0ueskug:0x

MaxMustermann2

Just one in-line comment, and if you could please merge peekpi#1 to fix the test.

And I believe this will need a config migration as well as change to config files in rosetta/infra if I am not wrong.

MaxMustermann2 · 2022-07-01T12:59:20Z

cmd/harmony/main.go

+			fromStr := strings.TrimSpace(substrings[0])
+			txSubstrings := strings.Split(substrings[1], ":")
+			toStr := strings.TrimSpace(txSubstrings[0])
+			dataStr := strings.TrimSpace(txSubstrings[1])


Why is data needed? Is the combination of from and to not enough?

Assuming data is needed, do we need any error checking for the dataStr ?

Is the data for smart contract call?

I don't think data is necessary, this feature is only for saving tokens from compromised users. All they should do is to send the token out to a new address.

Is the data for smart contract call?

yes, the ERC20 token transfer needs to call a smart contract.

I don't think data is necessary, this feature is only for saving tokens from compromised users. All they should do is to send the token out to a new address.

without data, users can only save ONE but ERC20.

Why is data needed? Is the combination of from and to not enough?

Assuming data is needed, do we need any error checking for the dataStr ?

yeah, you are right. It is better to add error checking

rlan35 · 2022-07-01T22:01:03Z

core/tx_pool.go

@@ -707,8 +719,15 @@ func (pool *TxPool) validateTx(tx types.PoolTransaction, local bool) error {
 		}
 		return ErrInvalidSender
 	}
+
+	inAllowedTxs := false


By default, if there is no entry for an address in allowedTx list, it should pass. Your current logic makes all txns by default being blacklisted. The correct logic should be

isNotAllowed := false if ......... check whether there is a entry for the sender address If Yes: check whether the to address matches: if Yes: isNotAllowed = false else isNotAllowed = true else isNotAllowed = false

IsNotAllowed == true is similar to being blacklisted.

inAllowedTxs indicated whether a tx/address is in the allowed tx whitelist. if it is, then skip to checking blacklist.

inAllowedTxs indicated whether a tx/address is in the allowed tx whitelist. if it is, then skip to checking blacklist.

otherwise, transactions will follow the original logic to check if it passes.

So you are saying this feature of allowedTxs have to work together with the blacklist to prevent hacker to steal token? I thought this feature can work on its own, which means if there is an entry in the allowedTxs, only those txns can be processed and all other txns from the same sender will be ignored.

yeah, we can make it work on its own. but if an address exists in the blacklist and whitelist, only the whitelist takes effect.

rlan35 · 2022-07-01T23:38:48Z

Please fix travis.

test: fix node tests for allowlist of txs

…llowedtxs

MaxMustermann2 · 2022-07-04T04:09:21Z

Can you add a config migration and add the flag to files in rosetta/infra ?

MaxMustermann2 · 2022-07-04T20:50:36Z

core/tx_pool.go

 	// Make sure transaction does not have blacklisted addresses
-	if _, exists := (pool.config.Blacklist)[from]; exists {
+	if _, exists := (pool.config.Blacklist)[from]; exists && !inAllowedTxs {
 		if b32, err := hmyCommon.AddressToBech32(from); err == nil {
 			return errors.WithMessagef(ErrBlacklistFrom, "transaction sender is %s", b32)
 		}


nitpick: Can you change the error string of ErrBlacklistFrom and print the data and to values in the message for better logging?

MaxMustermann2 · 2022-07-04T20:51:36Z

We are not going to change this at all, correct?

harmony/core/tx_pool.go

Lines 737 to 744 in d52a963

    
           if tx.To() != nil { 
        
           	if _, exists := (pool.config.Blacklist)[*tx.To()]; exists { 
        
           		if b32, err := hmyCommon.AddressToBech32(*tx.To()); err == nil { 
        
           			return errors.WithMessagef(ErrBlacklistTo, "transaction receiver is %s", b32) 
        
           		} 
        
           		return ErrBlacklistTo 
        
           	} 
        
           }

peekpi · 2022-07-05T06:15:01Z

We are not going to change this at all, correct?

harmony/core/tx_pool.go

Lines 737 to 744 in d52a963

if tx.To() != nil {

if _, exists := (pool.config.Blacklist)[*tx.To()]; exists {

if b32, err := hmyCommon.AddressToBech32(*tx.To()); err == nil {

return errors.WithMessagef(ErrBlacklistTo, "transaction receiver is %s", b32)

}

return ErrBlacklistTo

}

}

yeah, whitelisted transactions should not transfer to blacklisted addresses.

rlan35 · 2022-07-07T18:31:24Z

Looks good. Please do more testing on localnet or testnet to make sure it works as expected, since the malfunctioning of this feature could potentially cause real money loss.

peekpi · 2022-07-08T08:48:57Z

Looks good. Please do more testing on localnet or testnet to make sure it works as expected, since the malfunctioning of this feature could potentially cause real money loss.

yeah, it works as expected on localnet.

sophoah · 2022-07-14T17:16:13Z

@peekpi @MaxMustermann2 can we work on testing that feature all the time as part of the CI CD, it's a critical feature and we need to make sure it works all the time.

cc @LeoHChen

MaxMustermann2 · 2022-07-18T09:23:04Z

Local tests indicate that the config migration works.

Old config version detected 2.5.3
Do you want to update config to the latest version: [y/N]
y
Original config backed up to harmony.conf.backup
Successfully migrated harmony.conf from 2.5.3 to 2.5.4

Using a missing allow list file produces the following logs, which works well.

{"level":"debug","caller":"/home/user/go/src/github.com/harmony-one/harmony/cmd/harmony/main.go:892","time":"2022-07-18T14:12:15.711589879+05:30","message":"Using AllowedTxs file at `./.hmy/allowedtxs.txt`"}
{"level":"warn","caller":"/home/user/go/src/github.com/harmony-one/harmony/cmd/harmony/main.go:673","time":"2022-07-18T14:12:15.711629837+05:30","message":"AllowedTxs setup error: open ./.hmy/allowedtxs.txt: no such file or directory"}
{"level":"warn","caller":"/home/user/go/src/github.com/harmony-one/harmony/core/tx_pool.go:227","time":"2022-07-18T14:12:15.761365002+05:30","message":"Sanitizing nil allowedTxs set"}

Using the below allowlist file...

one155jp2y76nazx8uw5sa94fr0m4s5aj8e5xm6fu3->0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f:0x

...produces these logs.

{"level":"debug","caller":"/home/user/go/src/github.com/harmony-one/harmony/cmd/harmony/main.go:892","time":"2022-07-18T14:20:50.514788554+05:30","message":"Using AllowedTxs file at `./.hmy/allowedtxs.txt`"}

Sending a transaction from one155jp2y76nazx8uw5sa94fr0m4s5aj8e5xm6fu3 (without adding it to the blacklist) produces these logs:

{"level":"info","port":"9000","ip":"127.0.0.1","fullhash":"0x0d34f15cc23da82a9325be5bfb529cfe5afa3de971c20e453c0eb36f49383458","hashByType":"0x99bbb4fceb94cdd6d857d25e5b63b6cad3dfb0c93da0e64744ee8b0f9e107494","recipient":"0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f","tx":{"nonce":"0x1","gasPrice":"0x174876e800","gas":"0x5208","shardID":0,"toShardID":0,"to":"0x7a6ed0a905053a21c15cb5b4f39b561b6a3fe50f","value":"0x0","input":"0x","v":"0xc6afa5e4","r":"0x88ea6bab9d0a4f717c12963f6dbccc8180775a9cae4673949e73e7530ae39d0f","s":"0x128b4ff7159240dedebe59beaeb8f0cceb16b1ade10841ebcbd44648fe736fdc","hash":"0x0d34f15cc23da82a9325be5bfb529cfe5afa3de971c20e453c0eb36f49383458"},"caller":"/home/user/go/src/github.com/harmony-one/harmony/rpc/pool.go:132","time":"2022-07-18T14:45:59.003061756+05:30","message":"Submitted transaction"}

Adding one155jp2y76nazx8uw5sa94fr0m4s5aj8e5xm6fu3 as a line in the blacklist file and then sending a transaction to a non-allowlisted address produces this error:

>>> accounts[ 0 ].transfer( "0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE51f" )
  File "<console>", line 1, in <module>
  File "brownie/network/account.py", line 644, in transfer
    receipt, exc = self._make_transaction(
  File "brownie/network/account.py", line 752, in _make_transaction
    exc = VirtualMachineError(e)
  File "brownie/exceptions.py", line 96, in __init__
    raise ValueError(exc["message"]) from None
ValueError: transaction allowed whitelist check failed.

Sending the transaction to a allowlisted address but non-allowlisted data fails too.

>>> accounts[ 0 ].transfer( "0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f", data = "0xa" )
  File "<console>", line 1, in <module>
  File "brownie/network/account.py", line 644, in transfer
    receipt, exc = self._make_transaction(
  File "brownie/network/account.py", line 752, in _make_transaction
    exc = VirtualMachineError(e)
  File "brownie/exceptions.py", line 96, in __init__
    raise ValueError(exc["message"]) from None
ValueError: transaction allowed whitelist check failed.

But the allowlisted transaction goes through

>>> accounts[ 0 ].transfer( "0x7A6Ed0a905053A21C15cB5b4F39b561B6A3FE50f", data = "0x" )
Transaction sent: 0xaf3f41638ead59b6190c8edfd4239e1d336adf4396629121db83c86902955f82
  Gas price: 100.0 gwei   Gas limit: 21000   Nonce: 2
  Transaction confirmed   Block: 158   Gas used: 21000 (100.00%)

<Transaction '0xaf3f41638ead59b6190c8edfd4239e1d336adf4396629121db83c86902955f82'>

…mony-one#4218) * [txpool] fix harmony-one#4215, support allowed transaction list. * test: fix node tests for allowlist of txs * add error checking of tx.data * recover test scripts * config migration * add tx.data to error info * [pool] refactor: log more if `from` in denylist * reject tx if it does not pass the allowed whiltelist check Co-authored-by: MaxMustermann2 <82761650+MaxMustermann2@users.noreply.github.com>

[txpool] fix harmony-one#4215, support allowed transaction list.

28c16e4

peekpi requested review from MaxMustermann2, LeoHChen and rlan35 July 1, 2022 10:26

test: fix node tests for allowlist of txs

bf64234

MaxMustermann2 reviewed Jul 1, 2022

View reviewed changes

rlan35 reviewed Jul 1, 2022

View reviewed changes

peekpi and others added 4 commits July 2, 2022 12:25

Merge pull request #1 from MaxMustermann2/allowedtxs

0c98cab

test: fix node tests for allowlist of txs

add error checking of tx.data

1bc29b7

Merge branch 'allowedtxs' of https://github.com/peekpi/harmony into a…

e33aa93

…llowedtxs

recover test scripts

c24a05c

peekpi requested review from MaxMustermann2 and rlan35 July 3, 2022 15:52

config migration

d52a963

MaxMustermann2 reviewed Jul 4, 2022

View reviewed changes

add tx.data to error info

915944e

peekpi requested a review from MaxMustermann2 July 5, 2022 06:18

[pool] refactor: log more if from in denylist

bcc5efd

MaxMustermann2 approved these changes Jul 5, 2022

View reviewed changes

reject tx if it does not pass the allowed whiltelist check

c48d618

rlan35 approved these changes Jul 7, 2022

View reviewed changes

rlan35 merged commit 7ae12b1 into harmony-one:main Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[txpool] fix #4215, support allowed transaction list. #4218

[txpool] fix #4215, support allowed transaction list. #4218

peekpi commented Jul 1, 2022 •

edited

MaxMustermann2 left a comment

MaxMustermann2 Jul 1, 2022 •

edited

LeoHChen Jul 1, 2022

rlan35 Jul 1, 2022

peekpi Jul 2, 2022 •

edited

peekpi Jul 2, 2022

peekpi Jul 2, 2022

rlan35 Jul 1, 2022 •

edited

peekpi Jul 2, 2022

peekpi Jul 4, 2022 •

edited

rlan35 Jul 5, 2022 •

edited

peekpi Jul 7, 2022

rlan35 commented Jul 1, 2022

MaxMustermann2 commented Jul 4, 2022

MaxMustermann2 Jul 4, 2022

MaxMustermann2 commented Jul 4, 2022

peekpi commented Jul 5, 2022

rlan35 commented Jul 7, 2022

peekpi commented Jul 8, 2022

sophoah commented Jul 14, 2022

MaxMustermann2 commented Jul 18, 2022

[txpool] fix #4215, support allowed transaction list. #4218

[txpool] fix #4215, support allowed transaction list. #4218

Conversation

peekpi commented Jul 1, 2022 • edited

Issue

Config

cmd flag

config file

AllowedTxFile format

example

MaxMustermann2 left a comment

Choose a reason for hiding this comment

MaxMustermann2 Jul 1, 2022 • edited

Choose a reason for hiding this comment

LeoHChen Jul 1, 2022

Choose a reason for hiding this comment

rlan35 Jul 1, 2022

Choose a reason for hiding this comment

peekpi Jul 2, 2022 • edited

Choose a reason for hiding this comment

peekpi Jul 2, 2022

Choose a reason for hiding this comment

peekpi Jul 2, 2022

Choose a reason for hiding this comment

rlan35 Jul 1, 2022 • edited

Choose a reason for hiding this comment

peekpi Jul 2, 2022

Choose a reason for hiding this comment

peekpi Jul 4, 2022 • edited

Choose a reason for hiding this comment

rlan35 Jul 5, 2022 • edited

Choose a reason for hiding this comment

peekpi Jul 7, 2022

Choose a reason for hiding this comment

rlan35 commented Jul 1, 2022

MaxMustermann2 commented Jul 4, 2022

MaxMustermann2 Jul 4, 2022

Choose a reason for hiding this comment

MaxMustermann2 commented Jul 4, 2022

peekpi commented Jul 5, 2022

rlan35 commented Jul 7, 2022

peekpi commented Jul 8, 2022

sophoah commented Jul 14, 2022

MaxMustermann2 commented Jul 18, 2022

peekpi commented Jul 1, 2022 •

edited

MaxMustermann2 Jul 1, 2022 •

edited

peekpi Jul 2, 2022 •

edited

rlan35 Jul 1, 2022 •

edited

peekpi Jul 4, 2022 •

edited

rlan35 Jul 5, 2022 •

edited